Skip to content

test: prefer in-memory databases in sqlite tests - #64701

Open
bitpshr wants to merge 1 commit into
nodejs:mainfrom
bitpshr:test/sqlite-prefer-in-memory
Open

test: prefer in-memory databases in sqlite tests#64701
bitpshr wants to merge 1 commit into
nodejs:mainfrom
bitpshr:test/sqlite-prefer-in-memory

Conversation

@bitpshr

@bitpshr bitpshr commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Refs #64665. Converts the SQLite tests that only exercise SQL behavior (transactions, data types, typed-array/data-view binding) from temporary file databases to :memory:, which is faster and drops the nextDb() temp-file bookkeeping.

Tests that verify on-disk behavior (timeout, database-sync) intentionally keep using files. This is a focused first pass; the mixed-usage files (named-parameters, statement-sync) could follow.

Refs: #64665

cc @araujogui, let me know if this is what you had in mind, happy to extend it to the remaining files.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/sqlite

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jul 23, 2026
@codecov

codecov Bot commented Jul 23, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.27%. Comparing base (f9715fc) to head (3a2863c).
⚠️ Report is 170 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #64701      +/-   ##
==========================================
+ Coverage   90.14%   90.27%   +0.13%     
==========================================
  Files         741      762      +21     
  Lines      242194   247534    +5340     
  Branches    45606    46693    +1087     
==========================================
+ Hits       218323   223466    +5143     
- Misses      15358    15492     +134     
- Partials     8513     8576      +63     

see 172 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 24, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@geeksilva97 geeksilva97 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 24, 2026

@araujogui araujogui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we can extend the pattern to more test files

@araujogui

Copy link
Copy Markdown
Member

The only tests that need temporary files are DatabaseSync() constructor, open(), and backup().

@bitpshr

bitpshr commented Jul 24, 2026

Copy link
Copy Markdown
Contributor Author

Done, extended it per your guidance. Converted the named-parameter and statement suites plus the throwaway databases in the main test file to :memory:.

I kept files in a few places beyond the constructor/open/backup cases where an on-disk or shared database is actually required: the timeout suite and the cross-worker changeset test (both share a database by path), and the PRAGMA journal_mode = WAL test (WAL reports memory on an in-memory database). Let me know if you'd rather handle any of those differently.

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 26, 2026
@nodejs-github-bot

This comment was marked as outdated.

@araujogui araujogui left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@trivikr trivikr added the sqlite Issues and PRs related to the SQLite subsystem. label Aug 3, 2026
@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 3, 2026
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Aug 3, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator
Commit Queue failed
- Loading data for nodejs/node/pull/64701
✔  Done loading data for nodejs/node/pull/64701
----------------------------------- PR info ------------------------------------
Title      test: prefer in-memory databases in sqlite tests (#64701)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     bitpshr:test/sqlite-prefer-in-memory -> nodejs:main
Labels     test, author ready, needs-ci, sqlite
Commits    2
 - test: prefer in-memory databases in sqlite tests
 - test: convert more sqlite tests to in-memory
Committers 1
 - Paul Bouchon <mail@bitpshr.net>
PR-URL: https://github.com/nodejs/node/pull/64701
Refs: https://github.com/nodejs/node/issues/64665
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/64701
Refs: https://github.com/nodejs/node/issues/64665
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 23 Jul 2026 18:18:41 GMT
   ✔  Approvals: 2
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/64701#pullrequestreview-4781269372
   ✔  - Edy Silva (@geeksilva97): https://github.com/nodejs/node/pull/64701#pullrequestreview-4782351068
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2026-08-03T06:14:32Z: https://ci.nodejs.org/job/node-test-pull-request/75411/
- Querying data for job/node-test-pull-request/75411/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 64701
From https://github.com/nodejs/node
 * branch                  refs/pull/64701/merge -> FETCH_HEAD
✔  Fetched commits as 7e439828de88..a2aed4d77d9e
--------------------------------------------------------------------------------
[main 78f07cc15c] test: prefer in-memory databases in sqlite tests
 Author: Paul Bouchon <mail@bitpshr.net>
 Date: Thu Jul 23 14:10:32 2026 -0400
 3 files changed, 8 insertions(+), 35 deletions(-)
[main d90a2cdd97] test: convert more sqlite tests to in-memory
 Author: Paul Bouchon <mail@bitpshr.net>
 Date: Fri Jul 24 14:26:03 2026 -0400
 3 files changed, 46 insertions(+), 63 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
(node:374) [DEP0190] DeprecationWarning: Passing args to a child process with shell option true can lead to security vulnerabilities, as the arguments are not escaped, only concatenated.
(Use `node --trace-deprecation ...` to show where the warning was created)
Rebasing (2/4)
Executing: git node land --amend --yes
   ⚠  Found Refs: https://github.com/nodejs/node/issues/64665, skipping..
--------------------------------- New Message ----------------------------------
test: prefer in-memory databases in sqlite tests

Several SQLite tests created temporary file databases through a
nextDb() helper even though they only exercise SQL behavior and never
rely on filesystem persistence. Switch those to :memory:, which is
faster and drops the temporary-file bookkeeping. Tests that verify
on-disk behavior, such as the timeout and database-sync suites, keep
using files.

Refs: #64665
Signed-off-by: Paul Bouchon <mail@bitpshr.net>
PR-URL: #64701
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>

[detached HEAD e092714558] test: prefer in-memory databases in sqlite tests
Author: Paul Bouchon <mail@bitpshr.net>
Date: Thu Jul 23 14:10:32 2026 -0400
3 files changed, 8 insertions(+), 35 deletions(-)
Rebasing (3/4)
Rebasing (4/4)
Executing: git node land --amend --yes
⚠ Found Refs: #64665, skipping..
--------------------------------- New Message ----------------------------------
test: convert more sqlite tests to in-memory

Extend the change to the named-parameter and statement suites, which
only exercise SQL behavior, and to the throwaway databases in the main
test file. Tests that depend on an on-disk or shared database keep using
files: the constructor, open() and backup() cases, the timeout and
cross-worker suites, and the WAL journal-mode PRAGMA.

Refs: #64665
Signed-off-by: Paul Bouchon <mail@bitpshr.net>
PR-URL: #64701
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Edy Silva <edigleyssonsilva@gmail.com>

[detached HEAD df01704fcc] test: convert more sqlite tests to in-memory
Author: Paul Bouchon <mail@bitpshr.net>
Date: Fri Jul 24 14:26:03 2026 -0400
3 files changed, 46 insertions(+), 63 deletions(-)
Successfully rebased and updated refs/heads/main.

ℹ Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/30799387850

Several SQLite tests created temporary file databases through a
`nextDb()` helper even though they only exercise SQL behavior and never
rely on filesystem persistence. Switch those to `:memory:`, which is
faster and drops the temporary-file bookkeeping. Tests that depend on an
on-disk or shared database keep using files: the constructor, open() and
backup() cases, the timeout and cross-worker suites, and the WAL
journal-mode PRAGMA.

Refs: nodejs#64665
Signed-off-by: Paul Bouchon <mail@bitpshr.net>
@bitpshr
bitpshr force-pushed the test/sqlite-prefer-in-memory branch from a2aed4d to 3a2863c Compare August 3, 2026 19:54
@bitpshr

bitpshr commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author

Squashed into a single commit, so the earlier commit-queue "squash or rebase" ambiguity is resolved. Should be good to re-queue whenever.

@trivikr trivikr added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Aug 4, 2026
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 4, 2026
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. sqlite Issues and PRs related to the SQLite subsystem. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants